Skip to content

fix: Improve handling of escaped markers in apply_diff #2274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mrubens merged 4 commits into from
Apr 4, 2025
Merged

fix: Improve handling of escaped markers in apply_diff #2274

mrubens merged 4 commits into from
Apr 4, 2025

Conversation

@p12tic
Copy link
Contributor

@p12tic p12tic commented Apr 3, 2025 •
edited
Loading

Context

unescapeMarkers() is supposed to fix escaped merge conflict markers so that they match the code. However the function has a bug which requires SEARCH and REPLACE strings in the markers to be replaced. This is not part of merge conflict markers, so many valid cases were previously missed.

The change was requested in #2260 (comment).

Implementation

Adjusted replacement to be less strict.

Screenshots

Not applicable.

How to Test

Unit tests

Important

Fixes unescapeMarkers in multi-search-replace.ts to handle escaped markers without requiring specific strings, updating tests accordingly.

  • Behavior:
    • Fixes unescapeMarkers in multi-search-replace.ts to handle escaped markers without requiring 'SEARCH' or 'REPLACE'.
    • Adjusts regex in applyDiff to match new unescape logic.
  • Tests:
    • Updates multi-search-replace.test.ts to remove 'SEARCH' and 'REPLACE' from escaped markers in test cases.
    • Ensures tests validate correct processing of escaped markers without specific strings.

This description was created by Ellipsis for 4af2786. It will automatically update as commits are pushed.

@p12tic
fix: Improve handling of escaped markers in apply_diff
4af2786 
unescapeMarkers() is supposed to fix escaped merge conflict markers so
that they match the code. However the function has a bug which requires
SEARCH and REPLACE strings in the markers to be replaced. This is not
part of merge conflict markers, so many valid cases were previously
missed.
@p12tic p12tic requested review from cte and mrubens as code owners April 3, 2025 20:42
@changeset-bot
Copy link

changeset-bot bot commented Apr 3, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 63b29df

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 3, 2025
@p12tic p12tic mentioned this pull request Apr 3, 2025
Merged
@dosubot dosubot bot added the bug Something isn't working label Apr 3, 2025
@KJ7LNW
Copy link
Contributor

KJ7LNW commented Apr 4, 2025

Revert changes to existing tests, this change should not break tests I do not think.

If this change does require a test update, then please explain what happened and why...

@KJ7LNW
Copy link
Contributor

KJ7LNW commented Apr 4, 2025

also if you have a good argument for making changes to the tests, then I am listening, but I like to be conservative making changes to tests especially when a code change should not affect them.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 4, 2025
@mrubens
Copy link
Collaborator

mrubens commented Apr 4, 2025

I want to get this into tonight's release, so I pushed a change to revert changes to existing tests and to add a new test specific to this functionality. Hopefully not stepping on toes. Thanks for the work on this!

@mrubens mrubens force-pushed the fix-unescape-markers branch from fcc79dc to 63b29df Compare April 4, 2025 04:41
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 4, 2025
@mrubens mrubens merged commit 50eed96 into RooCodeInc:main Apr 4, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 4, 2025
@p12tic p12tic deleted the fix-unescape-markers branch April 4, 2025 10:58
@p12tic
Copy link
Contributor Author

p12tic commented Apr 4, 2025

@mrubens I think the test changes were needed because the crux of the fix is that more generic escaped markers are accepted. We have many tests that check escaped <<<<<<< SEARCH markers, where the correct check should be for whether <<<<<<< any text works (my tests used nothing for "any text").

@KJ7LNW
Copy link
Contributor

KJ7LNW commented Apr 4, 2025

I want to get this into tonight's release, so I pushed a change to revert changes to existing tests and to add a new test specific to this functionality.

Perfect, that is what I was looking for: keep existing tests and add new specific tests. If everything passes, then I'm good w/ the changes.

Thanks @p12tic for getting this prepared!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrubens mrubens mrubens approved these changes

@cte cte Awaiting requested review from cte cte is a code owner

Assignees

No one assigned

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@p12tic @KJ7LNW @mrubens